Reviewed 40 of 40 files at r1, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @lambda7xx)

lib/utils/include/utils/graph/adjacency_openmultidigraph.h line 66 at r1 (raw file):

};

// CHECK_NOT_ABSTRACT(AdjacencyOpenMultiDiGraph);

Why comment out this?

lib/utils/include/utils/graph/algorithms.h line 26 at r1 (raw file):

std::vector<Node> add_nodes(DiGraph &, int);
std::vector<Node> add_nodes(MultiDiGraph &, int);
std::vector<Node> add_nodes(MultiDiGraphView &, int);

I don't think add_nodes can apply to MultiDiGraphView.

lib/utils/include/utils/graph/algorithms.h line 113 at r1 (raw file):

                                                   Node const &);

std::unordered_set<MultiDiEdge> get_incoming_edges(MultiDiGraph const &,

We don't need this now since MultiDiGraph can coerce to MultiDiGraphView.

lib/utils/include/utils/graph/algorithms.h line 126 at r1 (raw file):

std::unordered_set<MultiDiEdge> get_incoming_edges(MultiDiGraphView const &,
                                                   std::unordered_set<Node>);
std::unordered_set<MultiDiEdge> get_incoming_edges(MultiDiGraph const &,

We don't need this now since MultiDiGraph can coerce to MultiDiGraphView.

lib/utils/include/utils/graph/digraph.h line 38 at r1 (raw file):

  IDiGraphView &get_ptr() const;

  // friend struct GraphInternal;

Remove this line

lib/utils/include/utils/graph/digraph.h line 73 at r1 (raw file):

  IDiGraph &get_ptr() const;

  // friend struct GraphInternal;

Remove this line

lib/utils/include/utils/graph/open_graph_interfaces.h line 19 at r1 (raw file):

      query_edges(OpenMultiDiEdgeQuery const &) const = 0;
  virtual std::unordered_set<MultiDiEdge>
      query_edges(MultiDiEdgeQuery const &) const = 0;

Why?

lib/utils/include/utils/graph/labelled/labelled_open.decl.h line 93 at r1 (raw file):

  void add_edge(MultiDiEdge const &e, EdgeLabel const &l);
  // void add_edge(InputMultiDiEdge const &e, EdgeLabel const &l);

Why?

lib/utils/include/utils/graph/labelled/node_labelled.h line 124 at r1 (raw file):

  }

  NodeLabelIf &get_nodelabel_ptr() const {

I prefer not to having this method. The reason we are using get_ptr() is to cast GraphView::ptr to the correct type. But we don't need that for nl since it is already in the right type.

lib/utils/include/utils/graph/labelled/output_labelled.h line 144 at r1 (raw file):

        ol(ol) {
  } // this exists some problem, interface is IMultiDiGraph, but
    // OutputLabelledMultiDiGraphView needs  IOutputLabelledMultiDiGraphView

We have decoupled graph structures and graph labels, so IOutputLabelledMultiDiGraphView has gone.

lib/utils/include/utils/graph/labelled/unordered_labelled_graphs.h line 156 at r1 (raw file):

  std::unordered_map<InputMultiDiEdge, InputLabel> input_map;
  std::unordered_map<OutputMultiDiEdge, OutputLabel> output_map;
  std::unordered_map<MultiDiEdge, EdgeLabel> edge_map;

I don't think we need edge_map since it is included in base_graph?

lib/utils/src/graph/adjacency_openmultidigraph.cc line 154 at r1 (raw file):

AdjacencyOpenMultiDiGraph *AdjacencyOpenMultiDiGraph::clone() const {
  NOT_IMPLEMENTED(); // TODO

Why? Have you run into any issues with this?

lib/utils/src/graph/views.cc line 539 at r1 (raw file):

ClosedMultiDiSubgraphView *ClosedMultiDiSubgraphView::clone() const {
  // return new ClosedMultiDiSubgraphView(g, nodes);
  NOT_IMPLEMENTED(); // TODO

Why?

lib/utils/test/src/test_labell_open.cc line 1 at r1 (raw file):

// #include "test/utils/all.h"

Why do you comment out all of this file?

lib/utils/test/src/test_openmultidigraph.cc line 1 at r1 (raw file):

// #include "test/utils/all.h"

Why?

I have reviewed the graph stuffs. Could you help review the others, i.e., fmt and stack stuffs? @lockshaw

Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @lambda7xx)

@wmdi I think they should all be resolved now.

FYI @lambda7xx: To free you up for more runtime-related tasks, @wmdi will be taking over finishing off and merging this PR

Reviewed all commit messages.
Reviewable status: all files reviewed, 26 unresolved discussions (waiting on @lambda7xx)

lib/utils/include/utils/fmt.h line 95 at r1 (raw file):

    ::std::unordered_map<Key, T, Hash, KeyEqual, Allocator> const &m,
    FormatContext &ctx) -> decltype(ctx.out()) {
  std::string result = "1";

Why?

lib/utils/include/utils/fmt.h line 114 at r1 (raw file):

                                          FormatContext &ctx)
    -> decltype(ctx.out()) {
  return formatter<std::string>::format(fmt::to_string(p.first), ctx);

Both elements of the pair should be present in the output: should be outputted as "(1, 2)" or something similar

lib/utils/include/utils/stack_string.h line 107 at r1 (raw file):

namespace fmt {

template <typename Char, std::size_t MAXSIZE>

Convert to format_as

lib/utils/include/utils/stack_vector.h line 331 at r1 (raw file):

} // namespace std

namespace fmt {

Convert to format_as

lib/utils/include/utils/vector.h line 4 at r1 (raw file):

#define _FLEXFLOW_UTILS_VECTOR_H

#include "utils/containers.h"

Why add an additional include here?

lib/utils/test/src/test_algorithms.cc line 45 at r1 (raw file):

  std::vector<Node> n = add_nodes(g, 4);
  std::vector<DirectedEdge> e = {
      // dst src

Remove (this seems likely to inevitably get out of date). If you think it's important to keep this for readability, use the syntax here so they can get auto-checked by clang-tidy

Also, I think it should be src, dst nor dst, src

lib/utils/test/src/test_algorithms.cc line 125 at r1 (raw file):

    CHECK(get_dfs_ordering(g, {n[0]}) ==
          std::vector<Node>{n[0], n[1], n[2], n[3]});
    CHECK(is_acyclic(g) == true); // maybe a bug about the

Make sure all of these bugs are fixed before merging

lib/utils/test/src/test_algorithms.cc line 138 at r1 (raw file):

  SUBCASE("nonlinear") {
    g.add_edge({n[1], n[3]});
    CHECK(is_acyclic(g) == false); // TODO, maybe a bug about the

Make sure bugs are fixed before merging

lib/utils/test/src/test_algorithms.cc line 220 at r1 (raw file):

  std::unordered_set<std::unordered_set<Node>> expected_components = {
      {n[1], n[2], n[0]}, {n[3]}};
  // get_connected_components should return {{n[1], n[2], n[0]}, {n[3]}, but it

Make sure bugs are fixed before merging

lib/utils/test/src/test_algorithms.cc line 239 at r1 (raw file):

  CHECK(get_outgoing_edges(as_digraph(as_undirected(g)), n[0]).size() == 1);
  // TODO: has some bug on get_weakly_connected_components

Make sure bugs are fixed before merging

lib/utils/test/src/test_bidict.cc line 3 at r1 (raw file):

#include "test/utils/doctest.h"
#include "utils/bidict.h"
#include "utils/containers.h"

Why add the additional include?

Reviewable status: all files reviewed, 26 unresolved discussions (waiting on @lambda7xx)

lib/utils/test/src/test_algorithms.cc line 45 at r1 (raw file):

Oh, @wmdi this is caused by inheriting in the order MultiDiEdge : MultiDiInput, MultiDiOutput instead of MultiDiEdge : MultiDiOutput, MultiDiInput. Can you fix this? This behavior of dst, src is rather unintutivie

No I am not working on this PR. I think I should have passed it to you...

Reviewable status: all files reviewed, 26 unresolved discussions (waiting on @lambda7xx and @lockshaw)

👍 I'll look into the code and we can discuss it on Friday

Reviewable status: all files reviewed, 26 unresolved discussions (waiting on @lambda7xx and @lockshaw)